Skip to content

Feat/issue 4899 simplify alignment for column geoms #4921

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 14 commits into from
Aug 23, 2022
Merged

Feat/issue 4899 simplify alignment for column geoms #4921

merged 14 commits into from
Aug 23, 2022

Conversation

@wurli
Copy link
Contributor

@wurli wurli commented Jul 26, 2022

This PR adds a just argument to geom_col() and geom_bar() (#4899) allowing a user to more easily change the alignment of columns. In particular, it makes it easy to align the sides of columns with axis breaks in cases where the resolution() of the data is not trivial to calculate.

  • just is used rather than hjust. This has a downside that it breaks with the hjust/vjust pattern used elsewhere in ggplot2, however it seems preferable since hjust would necessitate vjust for horizontal columns, which would raise difficult questions about whether to modify other behaviour, e.g. of has_flipped_aes().
  • There is a small improvement to the documentation for the width argument - resolution of the data now reads as resolution() of the data to more clearly hint at what's happening under the hood
  • This PR does not (as yet) include the extra aesthetics xmin and ymin for geom_col()

Here's how to use the just argument:

devtools::load_all()
#> i Loading ggplot2

df <- data.frame(
  x = 1:3,
  y = 1:3
)

ggplot(df, aes(y, x)) +
  geom_col(just = 1)

Created on 2022-07-26 by the reprex package (v2.0.1)

@yutannihilation
Copy link
Member

yutannihilation commented Jul 26, 2022 •
edited
Loading

Thanks. Could you avoid using the Windows line ending? If you are using RStudio, you can set it here (you might also need to review your Git settings as Git might convert the line endings on committing).

image

@wurli
Copy link
Contributor Author

wurli commented Jul 26, 2022

Thanks. Could you avoid using the Windows line ending? If you are using RStudio, you can set it here (you might also need to review your Git settings as Git might convert the line endings on committing).

Thanks, done now. I was puzzling about why the diff was showing all files as changed!

@yutannihilation yutannihilation added this to the ggplot2 3.4.0 milestone Jul 26, 2022
@yutannihilation yutannihilation mentioned this pull request Jul 26, 2022
Closed
yutannihilation
yutannihilation requested changes Jul 27, 2022
View reviewed changes
Copy link
Member

@yutannihilation yutannihilation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks almost good. Random comments:

  • I think we should add at least one example to show how just works briefly.
  • When used with some positions (e.g. position_dodge()), non-default just might result in a bit weird look. I'm wondering if we can add some friendly note about this.

@wurli
Copy link
Contributor Author

wurli commented Jul 27, 2022

Thanks, looks almost good. Random comments:

  • I think we should add at least one example to show how just works briefly.
  • When used with some positions (e.g. position_dodge()), non-default just might result in a bit weird look. I'm wondering if we can add some friendly note about this.

Both of these are now complete. The friendly note is in the documentation for the just argument itself, but may be more appropriate in the example. Not a big deal either way though 😄 Thanks!

@yutannihilation yutannihilation linked an issue Jul 27, 2022 that may be closed by this pull request
Simplify alignment for column geoms #4899
Closed
yutannihilation
yutannihilation reviewed Jul 27, 2022
View reviewed changes
Copy link
Member

@yutannihilation yutannihilation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! The friendly note looks good. Sorry, I failed to catch some more minor points. Also, could you resolve the conflict on NEWS.md?

@yutannihilation
Copy link
Member

Do you think the whole setup_data field for geom_col() and geom_bar() should have its own named function (e.g. setup_col_data()) to avoid code duplication btw?

I can't remember the reason why GeomBar and GeomCol are implemented separately, but, as now they are just duplicate. There's a plan to remove it (#3798), but, for now, could you just leave them duplicate? I think we should do one by one..

@yutannihilation yutannihilation mentioned this pull request Jul 27, 2022
Closed
yutannihilation
yutannihilation approved these changes Jul 27, 2022
View reviewed changes
Copy link
Member

@yutannihilation yutannihilation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! No further comments from my side.

@thomasp85
Could you also take a brief look when you have time?

@yutannihilation yutannihilation mentioned this pull request Aug 10, 2022
Merged
@thomasp85
Copy link
Member

LGTM

@yutannihilation
Copy link
Member

Thanks for reviewing!

@yutannihilation yutannihilation merged commit 8d0cef1 into tidyverse:main Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thomasp85 thomasp85 thomasp85 approved these changes

@yutannihilation yutannihilation yutannihilation approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

ggplot2 3.4.0

Development

Successfully merging this pull request may close these issues.

Simplify alignment for column geoms

3 participants

@wurli @yutannihilation @thomasp85